Emit header in MsgUpdateClient events #8624
Conversation
colin-axner
requested review from
aaronc,
alessio and
alexanderbez
as code owners
colin-axner
added
x/ibc
T: Dev UX
|
This looks like a non breaking fix for the v0.41 branch - correct @colin-axner? |
|
Tests are failing |
correct. Not urgent. It would just be very useful to include in the next point release, but no need to cut a point release just for this fix. It makes misbehaviour monitoring a lot easier |
ah yes, I will look into it |
…smos-sdk into colin/8598-update-header-event
|
fixed, though I have a few questions now and I must admit I don't fully understand the SDK backport policy. I'm not entirely sure how gas is calculated, but if this code increases the gas costs of a transaction, then it is possible a successful transaction on the old code, could run out of gas on the new code, causing a consensus failure. This seems like good reason not to backport this fix, but it also seems like most SDK fixes would fit this criteria. Maybe an extra if statement and var don't increase gas costs? |
Codecov Report
@@ Coverage Diff @@
## master #8624 +/- ##
=======================================
Coverage 61.38% 61.39%
=======================================
Files 672 672
Lines 38390 38404 +14
=======================================
+ Hits 23565 23577 +12
- Misses 12341 12342 +1
- Partials 2484 2485 +1
|
|
Is gas charged for emitting events, in a way which is dependent on the contents of an event (e.g. size)? I suppose it probably should be, but I'm not sure if it is. |
|
Based on the docs I read, it seems gas is only consumed for each read/write to the KVStore, in which case, this pr shouldn't change gas costs at all (since events aren't stored?) |
|
Not clear how the header is encoded, is this protobuf? Could we do the same as for |
Yes this is protobuf. Emitting packet data as protobuf requires changing how the packet data is encoded before it is hashed. We could do this, the data is never stored so I don't think gas costs should change. I also don't think it is IBC breaking even though the hash would change. It might be nice to be standardized about this. I think it would be good if ICS had a recommendation here so most applications use the same encoding format. thoughts @cwgoes ? cc @ethanfrey |
I actually meant NOT doing protobuf for the header but rather follow the same encoding as This is what I get for the update client header: and here is an example for the packet_data: |
|
@ancazamfir I can update it to use JSON |
…smos-sdk into colin/8598-update-header-event
|
Looks good now, thanks @colin-axner! |
|
One more thing, it looks like headers are not stored in IBC. So, on (re)start the relayer would have to get them from the Tx events. |
|
Awesome @ancazamfir! That's great to hear!
Yes, this is the unfortunate tradeoff made for the minimalist design of IBC, I think it falls into the same category as #8593. Perhaps one partial work around is to use the consensus state comparison on re(start) and only attempt to obtain the header from Tx events if the consensus states do not match. You'd still need to parse the tx events, but maybe it'd be a little more efficient. I think this is probably what the tendermint light client would need to do if it incorporates misbehaviour monitoring. Or perhaps the tendermint light client simply notifies the relayer that there is misbehaviour at height X and provides one of the misbehaving headers |
| // emitting events in the keeper emits for both begin block and handler client updates | ||
| ctx.EventManager().EmitEvent( | ||
| sdk.NewEvent( | ||
| types.EventTypeUpdateClient, | ||
| sdk.NewAttribute(types.AttributeKeyClientID, clientID), | ||
| sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()), | ||
| sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()), | ||
| sdk.NewAttribute(types.AttributeKeyHeader, headerStr), |
There was a problem hiding this comment.
this will emit an empty event value if the header is nil, is that expected?
There was a problem hiding this comment.
Yes, the header is only nil for the localhost client (which currently doesn't work). I cannot imagine a non-localhost client ever using a nil header. Nil headers should be disallowed following the refactor of the localhost client. I think emitting an empty event value is ok in the short term
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
colin-axner
added
the
A:automerge
* emit header in update client msg * update CHANGELOG * update spec * fix nil header bug * use JSON encoding for emitting header * Update x/ibc/core/spec/06_events.md * use proto for encoding * add tests * encode to hex before emitting header in event * add comment addressing reasoning for hex encoding * Update CHANGELOG.md Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> (cherry picked from commit a9b034b) # Conflicts: # CHANGELOG.md
(cherry picked from commit a9b034b) Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
(cherry picked from commit a9b034b) Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Description
cc @ancazamfir
closes: #8598
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes